Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

feat(demo): Update demo to work on OpenShift #2583

Merged
merged 1 commit into from
Mar 5, 2021
Merged

feat(demo): Update demo to work on OpenShift #2583

merged 1 commit into from
Mar 5, 2021

Conversation

ksubrmnn
Copy link
Contributor

@ksubrmnn ksubrmnn commented Feb 22, 2021 •
edited
Loading

Signed-off-by: Kalya Subramanian kasubra@microsoft.com

Description:
This PR updates the ports in the automatic and manual demos to use a "random" port that will not be reserved by OpenShift.

In the future, we can make the port configurable.

Fixes #2744

Affected area:

  • New Functionality [ ]
  • Documentation [ ]
  • Install [ ]
  • Control Plane [ ]
  • CLI Tool [ ]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests [ ]
  • CI System [ ]
  • Performance [ ]
  • Other [X]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?
    No

@ksubrmnn ksubrmnn added the wip Work-in-Progress label Feb 22, 2021
@codecov-io
Copy link

codecov-io commented Feb 22, 2021 •
edited
Loading

Codecov Report

Merging #2583 (d2ca8ea) into main (36b9159) will increase coverage by 0.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2583      +/-   ##
==========================================
+ Coverage   63.96%   64.10%   +0.13%     
==========================================
  Files         151      151              
  Lines        6755     6755              
==========================================
+ Hits         4321     4330       +9     
+ Misses       2419     2410       -9     
  Partials       15       15
Flag Coverage Δ
unittests 64.10% <ø> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/configurator/validating_webhook.go 76.55% <0.00%> (+6.20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36b9159...d2ca8ea. Read the comment docs.

@ksubrmnn ksubrmnn changed the title [WIP] feat(demo): Update demo to work on OpenShift feat(demo): Update demo to work on OpenShift Feb 24, 2021
shashankram
shashankram reviewed Feb 24, 2021
View reviewed changes
demo/cmd/common/books.go Outdated
@@ -19,7 +19,7 @@ const (
RestockWarehouseURL = "restock-books"

// bookstorePort is the bookstore service's port
bookstorePort = 80
bookstorePort = 8080
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is a work in progress, but we shouldn't be changing to default demo ports to 8080 just for this. If for some reason this port is unusable for Openshift, we should simply override the port similar to other configurable options.

In the past we did change the demo ports to 8080 and back to 80, more details can be found here #1099 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in favor of using a different port

@ksubrmnn ksubrmnn force-pushed the oc branch 2 times, most recently from 6a4119b to d2ca8ea Compare March 5, 2021 00:58
@ksubrmnn ksubrmnn removed the wip Work-in-Progress label Mar 5, 2021
@ksubrmnn ksubrmnn marked this pull request as ready for review March 5, 2021 17:45
@ksubrmnn ksubrmnn requested a review from a team as a code owner March 5, 2021 17:45
shashankram
shashankram approved these changes Mar 5, 2021
View reviewed changes
demo/deploy-bookbuyer.sh Show resolved Hide resolved
draychev
draychev approved these changes Mar 5, 2021
View reviewed changes
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll miss my goode olde port 80

@ksubrmnn
Copy link
Contributor Author

ksubrmnn commented Mar 5, 2021

I'll miss my goode olde port 80

To be addressed in #2755 :)

@ksubrmnn
feat(demo): Update automated demo to work on OpenShift
ba711be 
Signed-off-by: Kalya Subramanian <kasubra@microsoft.com>
@ksubrmnn ksubrmnn merged commit 3b7a6c7 into openservicemesh:main Mar 5, 2021
@ksubrmnn ksubrmnn deleted the oc branch March 5, 2021 22:57
@allenlsy allenlsy mentioned this pull request Jun 17, 2021
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shalier shalier shalier approved these changes

@snehachhabria snehachhabria snehachhabria approved these changes

@draychev draychev draychev approved these changes

@shashankram shashankram shashankram approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Automated demo does not work on OpenShift
7 participants
@ksubrmnn @codecov-io @shashankram @draychev @snehachhabria @shalier @nojnhuh